Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a basic MockServer integration test for Python #3534

Closed

Conversation

andreaTP
Copy link
Contributor

Reproducer for #3533

and cleaning the camp, since I'm here 🙂

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

@@ -98,7 +98,7 @@ httpx[http2]==0.25.0

hyperframe==6.0.1 ; python_full_version >= '3.6.1'

microsoft-kiota-abstractions==0.9.0
microsoft-kiota-abstractions==0.8.7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert this, I believe #3524 is going to fix this issue. (you could rebase onto that branch)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rebased

@andreaTP andreaTP changed the base branch from main to bugfix/snake-case-properties October 20, 2023 12:07
@andreaTP andreaTP changed the base branch from bugfix/snake-case-properties to main October 20, 2023 12:08
@baywet
Copy link
Member

baywet commented Oct 20, 2023

I think something went wrong during the rebase, the history contains a lot of commits and there's currently a conflict.

@andreaTP andreaTP changed the base branch from main to bugfix/snake-case-properties October 20, 2023 12:10
@andreaTP
Copy link
Contributor Author

yup, fixed now 👍 I'm targeting the bugfix/snake-case-properties branch with this PR, so that they can be merged together to main.

I hope this is fine for you and @samwelkanda

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HOLD merge until #3524 gets merged itself

@andreaTP
Copy link
Contributor Author

HOLD merge

@baywet I changed the target branch of this PR to be the one in #3524 , it's safe to be merged IMHO, should I keep it targeting main instead?

@baywet
Copy link
Member

baywet commented Oct 20, 2023

@andreaTP when #3524 gets merged, this one will automatically update its target to main (and keep the diff we currently have). My concern with merging this into #3524 right away is that it mixed concerns, especially if we need to make more changes there, and makes the reviews more difficult.

@andreaTP
Copy link
Contributor Author

Sounds good, thanks for explaining! 👍

@samwelkanda samwelkanda deleted the branch microsoft:bugfix/snake-case-properties October 24, 2023 13:57
@andreaTP
Copy link
Contributor Author

@samwelkanda @baywet I think that something went wrong here ...this should have been rebased, should I open it again?

@baywet
Copy link
Member

baywet commented Oct 24, 2023

interesting, the behaviour between PRs from the same repo and PRs across forks is different. Sorry about the mess here. Yes please do

@andreaTP
Copy link
Contributor Author

There is no "button" to re-open it from UI, I'll open a new PR from a different branch if this works for you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants